Skip to content

fix incorrect rounding of final weight volume #12970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fix incorrect rounding of final weight volume #12970

wants to merge 2 commits into from

Conversation

bouaik
Copy link
Contributor

@bouaik bouaik commented Nov 9, 2024

What? Why?

image
image

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

rioug
rioug approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍 Thanks @bouaik 🙏

dacook
dacook approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this and providing great documentation 📄 👍

db/migrate/20241101231144_change_final_weight_volume_to_scale_3_in_spree_line_items.rb Outdated
end

def down
change_column :spree_line_items, :final_weight_volume, :decimal, precision: 10, scale: 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that this action would be destructive.. but I guess if somebody was running the down action they would be expecting that.

@dacook dacook added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Nov 18, 2024
@filipefurtad0
Copy link
Contributor

Hey @bouaik,
Apologies we've taken so long to test this PR. In the meantime, some conflicts have arisen with master.

Are you available to address these?

Thank you.

@sigmundpetersen sigmundpetersen added the user facing changes Thes pull requests affect the user experience label Dec 4, 2024
@filipefurtad0
Copy link
Contributor

Moved this issue to In Progress so conflicts can be fixed.

@chahmedejaz chahmedejaz force-pushed the fix-incorrect-rounding-of-final-weight-volume branch from 39d845e to ba3de62 Compare March 17, 2025 22:23
@chahmedejaz
Copy link
Collaborator

Hi @filipefurtad0 - I've fixed the conflicts for this one, and it's ready for testing. Thanks.

@chahmedejaz chahmedejaz moved this from In Progress ⚙ to Test Ready 🧪 in OFN Delivery board Mar 17, 2025
@filipefurtad0 filipefurtad0 self-assigned this Mar 20, 2025
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Mar 20, 2025
@filipefurtad0
Copy link
Contributor

Hey @bouaik ,

Thanks for this fix.

Before this PR, we see the bug occuring:

Summary page
image

Order confirmation page
image

Order confirmation email
image

After this PR, we can see rounding occurs correctly:

image
image
image

We can also see this in the Orders and Fulfillment report, the previous order is marked in red / the new order is marked with the green dots:

image

Interestingly, the variant size does not appear in brackets for all variants, only for the 35mL like so (35mL). For the 125mL and for the 2mL this information is missing. Any idea why? @openfoodfoundation/train-drivers-product-owners do you think we should merge? The variant name is visible but the display unit seems missing, which might cause confusion to users. What do you think?

(Also, and out of scope: please notice, that changing the variant name also changes the naming in this report, for previously placed order. So it seems the information is being fetched from the product/variant names; wonder if the name should instead be fetched from the order instead.)

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Mar 20, 2025
@RachL
Copy link
Contributor

RachL commented Mar 20, 2025

@filipefurtad0 many thanks for this detailed testing, as always!

For the 125mL and for the 2mL this information is missing. Any idea why?

I could be wrong but maybe one is a display as and not the other? Otherwise weird indeed.

do you think we should merge? The variant name is visible but the display unit seems missing, which might cause confusion to users. What do you think?

If this can be fixed quickly within this PR, maybe that's the best, as it seems we would introduce this here. What do you think @bouaik ?

(Also, and out of scope: please notice, that changing the variant name also changes the naming in this report, for previously placed order. So it seems the information is being fetched from the product/variant names; wonder if the name should instead be fetched from the order instead.)

very good catch. We need indeed to keep the name on previous orders. Our users can sometimes completely change the product. Line items should never change when product catalog changes. Let's open a different issue for that one.

@filipefurtad0 filipefurtad0 mentioned this pull request Mar 20, 2025
Open
@sigmundpetersen sigmundpetersen moved this from Test Ready 🧪 to In Progress ⚙ in OFN Delivery board Mar 27, 2025
@sigmundpetersen
Copy link
Contributor

Hi @bouaik hope you are fine.
Did you want to follow up on the feedback here?

@github-project-automation github-project-automation bot moved this from In Progress ⚙ to Done in OFN Delivery board Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacook dacook dacook approved these changes

@rioug rioug rioug approved these changes

Assignees

@bouaik bouaik

@filipefurtad0 filipefurtad0

Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. feedback-needed user facing changes Thes pull requests affect the user experience
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent rounding truncates smallest digit for mL units
7 participants
@bouaik @filipefurtad0 @chahmedejaz @RachL @sigmundpetersen @dacook @rioug